-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: improvements to the oidc document #5063
base: main
Are you sure you want to change the base?
Conversation
👋 🤖 ✅ Looks like the changes were ported across versions, nice job! 🎉 You can read more about the versioning within our docs in our documentation guidelines. |
@hamza-m-masood Thank you for taking a look at this page! Can you add labels around which releases this should apply to? Right now the changes are only in 8.7, should they be mirrored in 8.8 (and updated later if 8.8 changes this process)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken an editing pass over this, but do want to confirm if it should also be included in 8.8, or backported to previous versions.
Are there other reviewers familiar with OIDC setup that might be valuable to add here?
@conceptualshark These changes need to be moved to 8.8 and 8.6. The guide for 8.8 will need to be reworked before being released anyway. To my knowledge, nobody has tested OIDC on 8.8 yet. I don't even think OIDC is supported on 8.8 yet. I just did not have the time to move the changes to other version since I'm busy with other tasks. |
@hamza-m-masood I've backported this to 8.6, and also moved it into 8.8 to keep things consistent. Whether or not it will be the same in the final state, it will keep the information available/consistent until we update it. @Ben-Sheppard Is this something you would be able to take a look at, or assign a reviewer who might be able to validate from the Identity end? Thank you! |
Hi @conceptualshark :) - I can validate the changes but it will be tomorrow if thats ok? |
Hi Folks, this came from a support engagement so I'd like to see that we make progress on this and get something merged and released soon. @Ben-Sheppard will you be able to review this ASAP (tomorrow, not now if you see this 😅 )? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of comments from my side but they are non blocking so I'll approve early :)
5. [Create a new client secret](https://learn.microsoft.com/en-gb/entra/identity-platform/quickstart-register-app?tabs=client-secret#add-credentials), and note the new secret's value for later use. | ||
6. Set the following environment variables for the component you are configuring an app for: | ||
:::note | ||
In Microsoft Entra ID, redirect URIs serve as an approved list of destinations. Only the URLs specified in the redirect URIs configuration will be permitted as valid redirection targets for authentication responses. This security measure ensures that tokens and authorization codes are only sent to pre-approved locations, preventing potential unauthorized access or token theft. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 I would suggest that this is a little too specific, the redirect URI concept is not just relevant to Entra but all auth providers
@@ -165,8 +176,8 @@ global: | |||
identity: | |||
clientId: <Client ID from Step 2> | |||
existingSecret: <Client secret from Step 5> | |||
audience: <Audience from Step 1> | |||
initialClaimName: <Initial claim name if not using the default "oid"> | |||
audience: <Audience from Step 2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ here we use step 2
but above we use step 3
, both points are related IMO so maybe step 2 and 3 could be combined?
Description
related: #4619 (comment)
When should this change go live?
bug
orsupport
label)available & undocumented
label)hold
label)low prio
label)PR Checklist
/docs
directory (version 8.8)./versioned_docs/version-8.7/
directory (version 8.7)./versioned_docs
directory.@camunda/tech-writers
unless working with an embedded writer.